feat(cache): add opt-in dependency snapshot store provider - #4461
feat(cache): add opt-in dependency snapshot store provider#4461kwakayama wants to merge 20 commits into
Conversation
…ed cache Staging preview health has been red since Sep 7: a fresh project renders its document against the pre-writeback dependency snapshot key, dependency writeback then rewrites package.json, and every replica that never resolved the old key answers pinned module requests with 409 Unknown dependency snapshot. The hydration runtime's static import graph dies on the first 409, so the page never defines __veryfrontRenderPage (veryfront-e2e preview-rendering.health.spec.ts, run 34250153773). The shared-snapshot machinery from #4451 solves exactly this, but nothing provided a DependencySnapshotStore in production, so history stayed process-local. Implement the store over the shared cache backends the module response caches already use (API cache or Redis; node-local disk and memory backends never qualify), and attach the process-wide handle at createHandlerDependencyPinningSource so document renders publish their snapshot and module requests on cold replicas recover it. Publication re-reads and verifies the written bytes because the backends fail open on set, and different bytes at a published key reject rather than overwrite, per the store contract. Claude-Session: https://claude.ai/code/session_01GNVuKWr64KLJRUrZvZJ3c4
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughAdds a cache-backed dependency snapshot store with bounded records, expiry checks, conflict handling, and revision-aware publication. Adds runtime adapter wiring for shared API or Redis backends. Tests cover recovery, isolation, failure handling, concurrency, and capability boundaries. ChangesDependency Snapshot Storage
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🔵 Low · up to Shared dependency snapshots enable cold-replica recovery, but one record-size enforcement path lacks direct test coverage. This is a bounded validation gap with low current production risk. Sequence Diagram(s)sequenceDiagram
participant RuntimeAdapter
participant DependencySnapshotStoreHandle
participant DependencySnapshotStore
participant SharedCacheBackend
RuntimeAdapter->>DependencySnapshotStoreHandle: provide configured store handle
DependencySnapshotStoreHandle->>DependencySnapshotStore: resolve shared backend
DependencySnapshotStore->>SharedCacheBackend: publish dependency snapshot
SharedCacheBackend-->>DependencySnapshotStore: verified retention
RuntimeAdapter->>DependencySnapshotStoreHandle: read snapshot on cold replica
DependencySnapshotStoreHandle->>DependencySnapshotStore: read namespace and key
DependencySnapshotStore->>SharedCacheBackend: bounded read
SharedCacheBackend-->>DependencySnapshotStore: stored snapshot
DependencySnapshotStore-->>RuntimeAdapter: dependency bytes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 41.18% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 7 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
📦 Client bundle boundary
A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
Review: 72/100 — solid root-cause fix with a real bounded-read gap against the store's own contractSummary: Correctly diagnoses and fixes the staging health-check regression (process-local snapshot history vs. replicated writeback), with a well-reasoned implementation and a genuinely good regression test — but the store bypasses this codebase's bounded-read mechanism, so it doesn't actually satisfy the "bound reads to 1 MiB" clause of the Strengths
Concerns
None of these block the core fix from working correctly for the staging incident described, so I'd call this conditionally approvable — worth addressing #1 before/soon after merge given it's an explicit, deliberately-documented invariant of the interface this PR implements. Generated by Claude Code |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79ec86dcae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🟡 Changes recommended
The new snapshot store implementation and its tests have contract/robustness gaps (expiry semantics, bounded reads, env-sensitive assertions) that should be corrected before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR addresses cross-replica hydration failures during dependency writeback by wiring a shared DependencySnapshotStore into the handler dependency pinning source, so replicas can resolve historical dependency snapshot keys via distributed cache storage rather than relying on process-local history.
Changes:
- Implement a cache-backed
DependencySnapshotStoreand expose a process-wide shared handle when a distributed cache backend is available. - Wire the shared snapshot store handle into
createHandlerDependencyPinningSource, covering both publish (document render) and read (module/data requests) paths. - Add focused unit/regression tests for store semantics and cold-replica recovery after dependency writeback.
Verification
- Not run in this environment (no task runner available via tools).
- Recommended:
deno task test:file src/cache/dependency-snapshot-store.test.tsdeno task test:file src/server/handlers/utils/dependency-pinning-source.test.ts
File summaries
| File | Description |
|---|---|
| src/server/handlers/utils/dependency-pinning-source.ts | Wires the shared dependency snapshot store handle into handler-scoped dependency pinning sources. |
| src/server/handlers/utils/dependency-pinning-source.test.ts | Adds a cold-replica regression test to ensure pre-writeback keys can be recovered via shared history. |
| src/cache/dependency-snapshot-store.ts | Introduces a distributed-cache-backed implementation of DependencySnapshotStore plus a lazily memoized shared handle. |
| src/cache/dependency-snapshot-store.test.ts | Adds unit coverage for store round-trip/idempotency/error behavior and shared-handle behavior. |
Review details
Suppressed comments (3)
src/cache/dependency-snapshot-store.ts:98
- The publication verification re-read should also be bounded to 1 MiB (per the DependencySnapshotStore contract) so a corrupted/oversized cache entry rejects safely instead of being fully materialized.
const writtenRaw = await backend.get(cacheKey);
src/cache/dependency-snapshot-store.ts:107
- Reads should use CacheBackend.getWithinLimit when available to enforce the 1 MiB maximum payload requirement for dependency snapshot history records.
const raw = await backend.get(recordKey(namespace, key));
src/cache/dependency-snapshot-store.test.ts:108
- This assertion is env-sensitive (isApiCacheAvailable / isRedisConfigured). Without clearing/restoring relevant env keys, this test can fail if another suite configured a shared cache backend and did not restore the environment.
it("returns undefined when no shared cache backend is configured", () => {
assertEquals(getSharedDependencySnapshotStoreHandle(), undefined);
});
- Files reviewed: 4/4 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Address the local codex review of #4461: - Reject node-local fallback backends inside the accessor factory so a Redis outage at initialization keeps the failure-retry path armed instead of pinning memory or disk as shared history for the life of the process. - Defer to an adapter that configures its own dependencySnapshotStore (captured absence included); the cache-backed default applies only when the adapter says nothing. - Verify the retained deadline, not just the bytes, after publication, with a small slack so concurrent same-value publishers acknowledge either write order while a silently dropped renewal still rejects. - Publish through getWithRevision/compareExchange when the backend exposes the revision capability, so conflicting concurrent publications acknowledge at most one winner; the read-back-verified path remains for backends without it. - Bound every record read (getWithinLimit where available, post-hoc byte assertion elsewhere) and enforce the 1 MiB payload limit before decoding or publishing. Claude-Session: https://claude.ai/code/session_01GNVuKWr64KLJRUrZvZJ3c4
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
@codex review |
Second local review round on #4461: - Local projects keep process-local history: a CLI-authenticated dev process can satisfy the shared-backend predicates without a cache-authorized tenant context, and a failing publication would break local rendering. - Size the record bound for JSON escaping: payloads are JSON text, so worst-case embedding doubles quotes and backslashes; twice the 1 MiB payload bound plus envelope admits every valid payload. Near-limit escape-dense round-trip pinned. - Document why the non-revisioned publish path's residual write race never surfaces legitimate divergence: snapshot bytes are the canonical serialization of exactly the state hashed into the key, so concurrent publishers at one key carry identical bytes; the checks still catch corruption, and backends that gain the revision capability get atomic publication automatically. Claude-Session: https://claude.ai/code/session_01GNVuKWr64KLJRUrZvZJ3c4
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 855c8cb69b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…tion Address PR review feedback and the CI failures on #4461: - Honor the optional AbortSignal: operations reject promptly at entry and between backend round-trips. Backend calls themselves are not cancelable, matching the registry's advisory-cancellation model. - read() returns null for a record a backend retained past its deadline; the contract reserves null for missing or expired history. - Move environment-dependent tests (pinning rollout flags, shared-backend predicates) to tests/integration/server/dependency-snapshot-store-wiring.test.ts with explicitly pinned env, keeping the unit files hermetic. This fixes the semantic-disposition lint and the env-sensitive coverage-shard assertions. - Document why the 60s renewal-deadline slack is safe: shared history can expire at most a minute early, and the registry renews half a retention period before expiry. - Format. Claude-Session: https://claude.ai/code/session_01GNVuKWr64KLJRUrZvZJ3c4
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
All review feedback is addressed as of eb367da. Disposition of each finding: Fixed in code
Resolved with rationale rather than code
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Reviewed eb367da and resolved five addressed threads: local-project isolation, adapter-store precedence, expired records, environment isolation, and the Gitar request to document non-atomic publication. The three focused suites pass all 30 steps. Codex also reports a clean review for this exact head. Seven threads remain open because their requested guarantees are still only partially implemented or explicitly deferred:
The author documented these tradeoffs. They remain inconsistent with the corresponding store-contract guarantees, so I have kept those threads open for an explicit contract change or implementation fix. |
|
Note Automatic reviews are paused because your trial's included automatic processing has been used for this period. Upgrade now, or comment "Gitar review" to run a review anytime. Code Review ✅ Approved 2 resolved / 2 findingsImplements the shared Store now honors the optional AbortSignal from the contract. Publication uses compare-and-set for atomic writes when backend revisions are available, with documented fallback for non-atomic backends. ✅ 2 resolved✅ Quality: Store ignores the optional AbortSignal from the contract
✅ Edge Case: publish read-modify-write is not atomic (TOCTOU)
OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
|
The latest head is One correction to the storage assessment: Redis has an optional atomic revision capability, advertised only after its topology/protocol probe succeeds. The current provider neither requires that capability nor uses the reserved revisioned key format, so simply reaching its optional CAS branch does not establish production correctness. The API backend has no revision capability. This reinforces the draft status and the need to test the actual qualifying backend path. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a050c8a540
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
The revision-capable path now consistently uses reserved revisioned cache keys, including verification and reads. The public factory and captured capability fixes are also verified. CI is green at The non-revisioned publication race remains open and this PR stays draft. The description records the remaining storage-contract work and separates this provider from the already merged staging fix. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a849878c6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
The latest review findings are fixed and their regression tests pass at @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cabeb13ce8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
The private-backend Promise species finding is fixed at @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1f53d5740
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| if (state.backend !== undefined) return Promise.resolve(state.backend); | ||
| if (state.backend !== undefined) { | ||
| return accessorApply(accessorPromiseResolve, AccessorPromise, [state.backend]) as Promise< |
There was a problem hiding this comment.
When project code installs a callable Object.prototype.then before a store operation, this native Promise.resolve(state.backend) performs thenable assimilation and invokes the inherited hook with the cached API or Redis backend as this; initialization's return b has the same behavior. This is separate from the fixed species path and exposes the opaque backend and its credential-bearing client, while the hook can also resolve null to make storage appear unavailable. Ensure promises carry only an opaque, non-thenable token rather than the raw backend.
AGENTS.md reference: AGENTS.md:L110-L112
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed at a1f53d5 with a deterministic cached-backend reproducer. After successful initialization, installing a callable Object.prototype.then exposes the exact synthetic API backend as the hook receiver on the next accessor call (1 exposure, expected 0), and the hook can replace the resolved result with null. Moving finally cleanup does not address native thenable assimilation. This requires an asynchronous capability contract that carries an opaque, non-thenable token instead of the raw backend, including the initialization path. I am leaving this finding open and keeping the PR draft alongside the atomic-publication blocker. The current Promise path is not ready to satisfy the claimed shared-realm private-storage contract.
|
Exact retention validation is fixed at Validation: 5 tests, 53 steps, plus explicit types, lint, formatting, semantic audit, and generated references. This does not resolve the atomic-publication or inherited-then backend-exposure findings, so both threads remain open and the PR stays draft. @codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@codex review Please review head 606eab3. It resolves the new main conflict by retaining the stricter Sharp cache-identity assertion. Sharp and cache/store suites pass 4 tests and 44 steps; formatting, semantic audit, generated references, and diff checks pass. The two confirmed storage blockers remain open, and the PR remains draft. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 606eab3372
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2edfe3f69
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review Please review head 2d3a7ce. It fixes the publication-deadline finding with three failing-then-passing regressions. The targeted store/deadline suites pass 2 tests (29 steps), and types, lint, formatting, semantic audit, and generated-reference checks pass. The two existing storage-contract blockers remain open, so this PR remains draft. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d3a7ce949
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
@codex review |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
This is an opt-in shared-cache implementation of
DependencySnapshotStore, building on #4451. A trusted host installscreateCacheDependencySnapshotStoreHandle()fromveryfront/platformon its runtime adapter before requests begin. The framework does not activate storage from ambient configuration.The original staging failure was a cold replica returning 409 for a dependency snapshot referenced by an older rendered document. #4459, now merged, addresses that incident through shared metadata-history recovery. This PR therefore needs to stand on its separate value: a reusable snapshot-store provider for explicitly configured hosts.
This PR remains draft because the current API and Redis publication path does not satisfy the store's immutable-key contract. A deterministic two-publisher interleaving reproduces two successful acknowledgements for different bytes at the same key. Read-back verification cannot make an unconditional write atomic across replicas. The existing CAS regression covers a synthetic revision-capable backend. Neither the API nor Redis implementation at this head or current main supplies getWithRevision/compareExchange. The snapshot provider now uses the reserved revisioned key format when the optional atomic capability is available. It still needs to require an atomic publication contract, distinguish backend failures from misses, and validate the actual qualifying production backend. Resolving this requires atomic storage support and corresponding production-backend tests. The unresolved review thread tracks that work.
The current implementation includes public opt-in wiring, validation, expiry handling, publication verification, and protection of private cache capabilities against the specific replaced-global hooks covered by tests. These checks are not evidence that all production-backend contract requirements are satisfied.
Validation at
2d3a7ce94932f1999a1c81a73deff1c3da7e212a, using Deno 2.7.7:deno task test:file src/cache/dependency-snapshot-store.test.ts tests/integration/cache/dependency-snapshot-publication-deadline.test.ts: 2 tests and 29 steps passed.The atomic-publication and inherited-then backend-exposure findings remain open. A deterministic probe confirms that a Promise resolution invokes an inherited then hook with the raw backend, even after the species fix. Completing private asynchronous storage requires an opaque, non-thenable capability transport across initialization and cache reuse. This is additional storage-contract work; the PR remains draft. Passing validation of the addressed findings does not establish production correctness or capability isolation.